fix(gui): share one chart timeframe across the Runtime Monitor tabs (#5496) - #5531
fix(gui): share one chart timeframe across the Runtime Monitor tabs (#5496)#5531skerker wants to merge 3 commits into
Conversation
The Memory tab and the Overview tab each built their own timeframe combo with the same four ranges, each read only by its own refresh, so the two could disagree: 1 hour chosen on Overview left Memory at 5 minutes. The Network Diagnostics dialog keeps one control in its page header and hides it on the pages with no chart. Move the combo to a header row above the tab widget, read it from one accessor in both refreshes, connect it to both so the tab that is not current redraws too, and hide the label and combo while Threads (fixed 60 s window) or Logs (no time axis) is current, comparing page pointers rather than tab indices. The object name `systemInfoTimeframe` and the accessible name are unchanged; `systemInfoOverviewTimeframe` is gone. system_info_dialog_test: the selector is the dialog's only combo box and a child of the body; hidden with Threads or Logs current, shown with Overview or Memory; the Overview tab has no selector of its own. Fixes aethersdr#5496. Refs aethersdr#2554, aethersdr#5427 (maintainer ruling item 3). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DonGWakahMZs3nbwAKdHzE
With the label and combo hidden on Threads and Logs, their row collapsed and the tab strip moved up about 32 px, so a click on "Threads" slid the strip out from under the pointer. Retain the widgets' size when hidden; the row stays put, the widgets stay hidden (isHidden() unchanged, the dialog test's visibility assertions still hold). Refs aethersdr#5496. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DonGWakahMZs3nbwAKdHzE
Keep only the two edits the existing target needs after aethersdr#5496: the selector label no longer names the Memory tab, and the Overview block asserts its own combo is gone instead of present. The added visibility and placement assertions are dropped — the bridge proof on the PR demonstrates every criterion, and a maintainer would trim them at merge. Refs aethersdr#5496. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DonGWakahMZs3nbwAKdHzE
skerker
changed the title
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Issue fit
Yes, on three of the four acceptance criteria, verified by reading the head checkout at 4b2ce8ca:
- One control in the header, none inside any tab —
SystemInfoDialog.cpp:227-262builds it above theQTabWidget; both per-tab combos are gone, andsystemInfoOverviewTimeframeno longer occurs anywhere in the tree (grepped: only the test's absence assertion). - Both refreshes read it —
selectedRangeSeconds()(:699) is the single reader, called fromrefreshMemoryChartandrefreshOverview. - Hidden on Threads and Logs —
:272-279, page-identity comparison rather than index. - Object/accessible names carry over —
systemInfoTimeframeand "Chart timeframe" preserved; the Memory test finds it unchanged.
The non-goals hold: nothing persists the choice, the four ranges and bucket rules are untouched, and the tab structure is unchanged. The shape matches NetworkDiagnosticsDialog.cpp:326-341 and :957-960 closely enough that I diffed them side by side — same label text, same object-name convention, same accessible description phrasing, same page != logsTab && page != tciTab idiom. The one deliberate divergence is setRetainSizeWhenHidden (commit 2), which the network dialog does not do; that is an improvement, not a drift.
Scope
| File | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/gui/SystemInfoDialog.cpp |
Adds the header row + visibility lambda; deletes both per-tab combos; renames selectedMemoryRangeSeconds → selectedRangeSeconds |
Yes | In scope |
src/gui/SystemInfoDialog.h |
m_overviewRange/m_memoryRange → m_rangeLabel/m_range; one accessor instead of two |
Yes | In scope |
tests/system_info_dialog_test.cpp |
Retargets the Memory lookup; converts the Overview assertion to an absence check | Yes | In scope, but see nit 1 |
Everything in the diff is explained by the issue. No new settings key, no protocol surface, no CHANGELOG entry, no unrelated files, no formatting churn. The deleted - lines are the two combos and their comments; neither comment names a symptom that can recur — they document the per-tab rationale that #5496 explicitly overrules. No sibling implementation left behind: NetworkDiagnosticsDialog already had the window-level control, and no third dialog carries a chart timeframe.
Preference check: this is a fix, not smuggled preference — the issue exists, carries a maintainer ruling from #5427, and names the reference behavior (the network dialog) the PR moves toward. No default changed: both old combos already defaulted to index 1, and the new one does too.
Socket tests: none added, modified, or removed. tests/system_info_dialog_test.cpp opens no socket and constructs no fake peer.
Blockers
None.
Nits (non-blocking)
-
The test would pass with the two
connectlines deleted. The Overview assertion is now purely negative (findChild(...) == nullptr), and the Memory case'srange->setCurrentIndex(3)only asserts the readouts did not change — which is true whether or notrefreshOverviewis wired. So the PR's central claim, "changing the shared control redraws the Overview charts too," has no coverage at any layer. I checked whether it could:TimeSeriesGraphWidget(src/gui/TimeSeriesGraphWidget.h:60) storesm_rangeSecondsprivately with no accessor, so asserting the redraw range would need a new getter — fair to leave out. But acceptance criterion 3 is cheap here and untested:findChild<QTabWidget*>(),show(),setCurrentIndex()to Threads, thenm_range->isVisible() == false. That is a socket-free assertion in an already-registered target. -
Both
m_rangeLabelandm_rangecarry the accessible name "Chart timeframe" (:231,:234). A screen reader passing over the row announces it twice. Copied verbatim from the network dialog, so this is a pre-existing convention rather than something this PR introduced — flagging only in case it's worth fixing in both places later.
What I tried to break
- Construction order.
refreshOverviewandrefreshMemoryChartare now reachable from the combo. I checked thatm_rangeis fully built at:227-262beforebuildOverviewTab()/buildMemoryTab()run at:264-267, and thatsetCurrentIndex(1)at:243precedes bothconnectcalls at:254-258— so no refresh fires against half-built graph pointers.selectedRangeSeconds()also keeps the old null guard returning 300. - Null pointers on the new path.
refreshOverviewdereferences fourm_overview*Graphpointers; every one is behind anif (... != nullptr), and the ring accessors are guarded bycpu != nullptr/mem != nullptr. Changing the combo before any sample has arrived draws empty series rather than crashing. - Dangling capture at teardown. The visibility lambda captures raw
tabs,threadsTab,logsTaband touchesm_rangeLabel/m_range. IfQTabWidget::currentChangedcould fire while children are being destroyed, those would be stale. Qt 6's~QObjecttears down connections beforedeleteChildren(), and the receiver context isthis, so the connection is gone beforetabsdies. Same shape the network dialog already ships. - Stale external references. Grepped the whole head checkout for
systemInfoOverviewTimeframe,m_overviewRange,selectedOverviewRangeSecondsandselectedMemoryRangeSecondsacross sources, tests, docs,resources/helpand the touchpoint manifests — the only survivor is the test's deliberate absence check. Nothing in the automation bridge orSupportBundleaddresses these widgets. - Tab identity vs index. The comparison is by page pointer, so the Painters tab that the
:263comment anticipates can be inserted anywhere without silently flipping the rule.buildOverviewTab()returns itsQScrollAreawrapper, and that same pointer is whataddTabreceives, socurrentWidget()compares consistently. - CI. All five checks green on
4b2ce8ca(build, check-macos, check-windows, Static checks, Sanitizer). Noting the standing caveat thatctestinci.ymlis-R-filtered, so green is not the full suite.
All of the above is reasoned from source in /tmp/aetherclaude/pr-5531; I am headless and did not build, run the dialog test, or drive the GUI, so no runtime behavior here was observed.
Recommendation
Approve with nits. The change does exactly what the issue asks, matches the reference dialog it cites, and I could not construct an ordering, lifetime, or null case that breaks it. The one thing worth asking for before merge is a visibility assertion in system_info_dialog_test.cpp — the criterion the PR's own commit message calls out is the one nothing currently pins, and it costs about four lines in a test that already exists.
🤖 aethersdr-agent · cost: $6.4786 · model: claude-opus-5
| auto* range = ov.findChild<QComboBox*>(QStringLiteral("systemInfoOverviewTimeframe")); | ||
| report("the Overview tab has its own timeframe selector", range != nullptr && range->count() == 4); | ||
| report("the Overview tab has no timeframe selector of its own (#5496)", | ||
| ov.findChild<QComboBox*>(QStringLiteral("systemInfoOverviewTimeframe")) == nullptr); |
There was a problem hiding this comment.
This is now the only Overview-side assertion about the timeframe, and it is purely negative — it passes as long as the old object name is absent. Combined with the Memory case (which asserts only that the readouts are undisturbed by setCurrentIndex(3)), deleting both connect lines in SystemInfoDialog.cpp:254-258 would leave the whole file green.
The redraw itself is genuinely hard to assert — TimeSeriesGraphWidget keeps m_rangeSeconds private with no getter — so I'm not asking for that. But the issue's third acceptance criterion is cheap in this already-registered socket-free target:
report("the Overview tab has no timeframe selector of its own (#5496)",
ov.findChild<QComboBox*>(QStringLiteral("systemInfoOverviewTimeframe")) == nullptr);
// ...and the shared one hides on the tabs with no chart (#5496).
auto* ovTabs = ov.findChild<QTabWidget*>();
auto* ovRange = ov.findChild<QComboBox*>(QStringLiteral("systemInfoTimeframe"));
if (ovTabs != nullptr && ovRange != nullptr) {
ov.show();
QCoreApplication::processEvents();
report("the timeframe shows on Overview", ovRange->isVisible());
ovTabs->setCurrentIndex(1); // Threads: fixed 60 s window
report("the timeframe hides on Threads", !ovRange->isVisible());
ovTabs->setCurrentIndex(2); // Memory: charted again
report("the timeframe returns on Memory", ovRange->isVisible());
ov.hide();
}Non-blocking.
| connect(m_range, &QComboBox::currentIndexChanged, this, | ||
| &SystemInfoDialog::refreshMemoryChart); | ||
| connect(m_range, &QComboBox::currentIndexChanged, this, | ||
| &SystemInfoDialog::refreshOverview); |
There was a problem hiding this comment.
Checked the ordering hazard this introduces and it holds: m_range is fully populated and setCurrentIndex(1) has already run by line 243, both connects come after it, and buildOverviewTab()/buildMemoryTab() only run at 264-267 — so neither slot can fire against null graph pointers during construction, and the default selection raises no spurious refresh. refreshOverview is null-guarded on all four m_overview*Graph members besides.
No change requested; recording it because "one combo now drives two refreshes" is the place this would have gone wrong.
1 participant
Summary
Fixes #5496. The Runtime Monitor's Memory tab and Overview tab each built their own chart timeframe combo with the same four ranges, each read only by its own refresh (
src/gui/SystemInfoDialog.cpp:583and:772onmain8a358c5f), so the two could disagree by construction: a range chosen on one tab never applied to the other. This moves the one control into a header row above the tab widget, the way the Network Diagnostics dialog keeps itsnetworkDiagnosticsTimeframein its page header (NetworkDiagnosticsDialog.cpp:327) and hides it on the pages with no chart (:958). Filed on the maintainer ruling in #5427 (design decision 3: accepted for that PR with this follow-up). Part of #2554.What changed, by acceptance criterion:
QTabWidget; both per-tab copies are deleted.m_memoryRange/m_overviewRangebecome onem_range.selectedRangeSeconds()replaces the two;currentIndexChangedis connected to bothrefreshMemoryChart()andrefreshOverview(), so the tab that is not current redraws too.currentChangedtoggles label and combo by comparing the current page pointer against the Threads and Logs pages, not by tab index (the order changed once already in feat(gui): Runtime Monitor Overview tab — cards and the four charts (#2554) #5427).systemInfoTimeframeand accessible name "Chart timeframe" are unchanged; the accessible description is now chart-neutral ("Choose how much recent history the charts display."). The Overview copy's object namesystemInfoOverviewTimeframe(shipped in feat(gui): Runtime Monitor Overview tab — cards and the four charts (#2554) #5427 on 2026-09-08) no longer exists.The two comments that justified the per-tab placement (
:570-574,:766-767) are rewritten. Non-goals per the issue: no persistence of the choice (the Network dialog does not persist its own), no change to the ranges or bucket rules, no sidebar grouping.Constitution principle honored
Principle XI — Fixes Are Demonstrated: every criterion is demonstrated through the agent automation bridge on the demo simulator, with the readings below. No test was added.
Test
Proof shape: bridge only. This is UI wiring — where a control lives and when it is shown — so the bridge readings and captures below are the proof, and no assertion was added.
tests/system_info_dialog_test.cpp(existing targetsystem_info_dialog_test) carries only the two edits the move forces: the Memory block's label no longer names the Memory tab (its lookup bysystemInfoTimeframeon the dialog is unchanged and still finds the control), and the Overview block asserts thatsystemInfoOverviewTimeframeis gone instead of present. Local: the target passes at4b2ce8ca(offscreen). It runs onfull-suite.ymlat merge and weekly onsanitizers.yml; it is not on the frozen per-PR gate (#5405).Proof
Agent automation bridge, demo simulator
DEMO-0001(familysim), RX only, isolated settings store,AETHER_AUTOMATION_NO_TX=1. Readings atfae75daf(Aboutv26.9.2 (fae75daf)); the Threads/Memory pair re-read at390ba3d6. The head4b2ce8cadiffers from390ba3d6intests/only, so the app binary is the same.dump_treefilter "Timeframe": oneQComboBoxsystemInfoTimeframe, parent = the dialog body, above the tab widget; filtersystemInfoOverviewTimeframe: 0 matches.visible: false. Logs current:visible: false. Back on Overview and Memory:visible: true.390ba3d6: the tab strip's geometry is the same with Memory current and with Threads current (the hidden row keeps its height).No "before" build was run; the pre-fix state is the source cited above.
Evidence bundle index:
fae75daf· About PNG, seven Runtime Monitor captures, the instance's whole log, ctest head and mutant outputs · aethersdr-pr5531-shared-timeframe-2026-09-09.zip (sha2563bc2ee1f…0461f)390ba3d6· About PNG, Memory and Threads captures with the tab strip at the same y, ctest run at4b2ce8ca· aethersdr-pr5531-retain-row-2026-09-09.zip (sha256e9f4e793…3e6f1)What I tried to break
fae75daf: hiding the row let it collapse, so the tab strip jumped about 32 px between Memory and Threads.390ba3d6retains the widgets' size while hidden; the row stays put.Not tested: a real radio (nothing in this change reads radio state); Linux and Windows builds (the code is plain Qt Widgets with no platform branch).
Test plan
cmake --build build) — clean RelWithDebInfo builds atfae75dafand390ba3d6(4b2ce8cais tests-only), 0 warnings in the touched filesChecklist
docs/COMMIT-SIGNING.md)AppSettingscalls — n/a, no settings touchedMeterSmoother— n/a, no metersdocs/has no mention of the Runtime Monitor timeframe;CHANGELOG.mduntouched per AGENTS.md— authored by agent (Claude Code) on behalf of @skerker